Codecov Report

Attention: Patch coverage is 91.81034% with 19 lines in your changes missing coverage. Please review.

Project coverage is 87.84%. Comparing base (21d8275) to head (1138a80).
Report is 1 commits behind head on main.

@@            Coverage Diff             @@
##             main     #102      +/-   ##
==========================================
+ Coverage   87.75%   87.84%   +0.09%     
==========================================
  Files          31       33       +2     
  Lines        1347     1456     +109     
==========================================
+ Hits         1182     1279      +97     
- Misses        165      177      +12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Wrong Gar(r)y tagged in the PR description @evalott100 ?

Wrong Gar(r)y tagged in the PR description @evalott100 ?

Hopefully, I'm too on holiday to give a review

Very mad that you replied Garry!!
Looks good at a first look, I'll try and run this against fastcs-odin or eiger. I can't remember if there's an issue for this in PVI or somewhere else, but I think for enums the ComboBox may only be set up to send the string value instead of the index to the handler, for odin we would have to find the associated index for that value in the Sender's put and send that via the http connection, which may be okay, depends on if we generally want to be able to have the dropdowns with different labels to the actual value that gets sent.

Wrong Gar(r)y tagged in the PR description ?

Apologies this is what I get for tying @G and expecting autocomplete to work while sleepy

depends on if we generally want to be able to have the dropdowns with different labels to the actual value that gets sent

Yes we do. The user wants to see the text label and doesn't care if the device gets sent the label or the index.

Yes we do. The user wants to see the text label and doesn't care if the device gets sent the label or the index.

This confuses me, the phoebus screen will be updating the IOC values (which would have an mbb record which takes indices or string), which will update the record through ParamTreeHandler::update.

What would need to change is where we make the attribute.

allowed_values is now an field on the DataType so if

1. You want a simple string with allowed values (what you have now), then types should map the datatype classes instead of instances and you can initialise them in _create_attributes with the allowed_values.

This would have the exact same behaviour across the stack so long as we change

so that strings with allowed values also make ComboBox (I would be in favour of this).

2. You want to use an Enum, then you create an enum at Runtime based on the labels given then you make quick cast_to_http/cast_from_http which get the name/value of the attribute's enum datatype and use them here and here respectively. The latter would be a bit more annoying without StrEnum, I'm thinking that it might be best to allow StrEnum too 🤔

strings with allowed values also make ComboBox (I would be in favour of this).

+1

you create an enum at Runtime based on the labels given then you make quick cast_to_http/cast_from_http which get the name/value of the attribute's enum datatype and use them here and here respectively.

I did try this at some point in a stale branch and it worked fine so we could keep all this logic inside fastcs-odin, though would be nice if it was resolved higher up. I'm not sure if Odin is unusual in how it handles enums though I suspect there are other potential use cases that would expect ints. (odin-control's ParameterTree doesn't actually properly implemented string labels for enums in its own allowed_values/metadata currently so I was manually adding in these labels for testing, I want to rework that at some point anyway, maybe this should be more of an Odin change)

though would be nice if it was resolved higher up

I agree. My initial approach was to add a cast method to the DataType itself, but that falls short because we have no way to know what cast logic the 'backend' would be require - e.g the restapi requires casting np.ndarray to python lists.

It might clean things up if we added a set_cast_method to DataType, then cast on the datatype either returns the value or uses the given cast method if it has one. This could fall short if the backend (e.g epics) expects one casted datatype, but a downstream 'backend' (e.g http in Odin, or pandablocks-client) expects another.

One solution

Upstream, we could have something like:

match datatype:
     case Enum(enum_cls):
         datatype.add_cast_to(
             SOME_EPICS_BACKEND_HASHABLE_IDENTIFIER,
             lambda enum: enum.value
         )
         datatype.add_cast_from(
             SOME_EPICS_BACKEND_HASHABLE_IDENTIFIER,
             lambda value: enum_cls(value)
         )

Then

becomes

 async def async_write_display(value: T): 
     record.set(attribute.datatype.cast_to(SOME_EPICS_BACKEND_HASHABLE_IDENTIFIER, value), process=False) 

and

becomes

 async def on_update(value): 
     await attribute.process_without_display_update(
         attribute.datatype.cast_from(SOME_EPICS_BACKEND_HASHABLE_IDENTIFIER, value)
     ) 

Then downstream you could

add_cast_from(
     SOME_HTTP_ODIN_BACKEND_HASHABLE_IDENTIFIER,
     lambda value: enum_cls(value)
)

Since the http client in odin implements it's own updater I'm unsure if it's a good idea though.

Could we do waveform in a separate PR? I have some comments on that, but I think we could get that in it quickly whereas there is a lot to discuss on enums.

https://github.com/evalott100/fastcs-eiger/tree/adjust-to-use-new-enum

@evalott100 we have merged #98 - could you resolve the conflicts? I would like to get this in next.